update(arc-basic): highcharts — new implementation#4374
Merged
github-actions[bot] merged 9 commits intomainfrom Feb 23, 2026
Merged
update(arc-basic): highcharts — new implementation#4374github-actions[bot] merged 9 commits intomainfrom
github-actions[bot] merged 9 commits intomainfrom
Conversation
New highcharts implementation for arc-basic (Basic Arc Diagram).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new highcharts implementation for the arc-basic plot type. The implementation uses Highcharts' arcdiagram series type to visualize character interactions in a story, showing dialogue exchanges as weighted connections between characters arranged along a horizontal axis.
Changes:
- Created new highcharts implementation using raw JSON configuration (since highcharts_core doesn't support arcdiagram type)
- Added metadata YAML file with basic structure for future quality review
- Implemented with Selenium-based screenshot generation and HTML export
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plots/arc-basic/metadata/highcharts.yaml | Metadata file for the highcharts implementation with library version, timestamps, and preview URLs |
| plots/arc-basic/implementations/highcharts.py | Complete highcharts implementation using arcdiagram series type with data generation, styling, and rendering |
Comments suppressed due to low confidence (5)
plots/arc-basic/implementations/highcharts.py:273
- Debug JavaScript code is left in the implementation (lines 255-273). This code prints node information to the console and was likely used during development. Debug code should be removed before merging to production as it adds unnecessary execution time and console output.
# Debug: check SVG node elements
node_info = driver.execute_script("""
var chart = Highcharts.charts[0];
if (!chart || !chart.series[0]) return 'no chart/series';
var nodes = chart.series[0].nodes;
if (!nodes) return 'no nodes property';
var info = 'nodes: ' + nodes.length;
nodes.forEach(function(n, i) {
if (n.graphic) {
var el = n.graphic.element;
info += '\\n ' + i + ': tag=' + el.tagName + ' attrs=';
for (var a of el.attributes) info += a.name + '=' + a.value + ' ';
} else {
info += '\\n ' + i + ': no graphic';
}
});
return info;
""")
print(node_info)
plots/arc-basic/implementations/highcharts.py:73
- The weight values are scaled up by a factor of 8 (line 72), but this appears arbitrary and the comment suggests this is to work around how arc diagram sizes nodes. Consider if there's a more direct way to configure node sizing in Highcharts rather than manipulating the underlying data values, as this makes the data representation less transparent.
# Scale weights up for visual node sizing (arc diagram sizes nodes by total weight flow)
weight_scale = 8
links_data = [{"from": src, "to": tgt, "weight": w * weight_scale} for src, tgt, w in edges]
plots/arc-basic/implementations/highcharts.py:199
- The implementation includes complex post-render DOM manipulation JavaScript (lines 173-199) that removes link labels and modifies node circles. This suggests the Highcharts arcdiagram configuration isn't fully meeting requirements declaratively. While this may be necessary given library limitations, it increases maintenance complexity and fragility. Consider documenting why this workaround is needed and whether there are alternative Highcharts configuration options that could reduce or eliminate the need for DOM manipulation.
cleanup_js = """
setTimeout(function() {
// Remove link labels (internal IDs rendered on arc paths)
document.querySelectorAll('.highcharts-data-label textPath').forEach(function(el) {
el.parentNode.parentNode.style.display = 'none';
});
document.querySelectorAll('.highcharts-data-label text').forEach(function(el) {
if (el.textContent && el.textContent.indexOf('highcharts-') === 0) {
el.parentNode.style.display = 'none';
}
});
// Enlarge node circles for 4800x2700 canvas
var chart = Highcharts.charts[0];
if (chart && chart.series[0] && chart.series[0].nodes) {
chart.series[0].nodes.forEach(function(node) {
if (node.graphic) {
var el = node.graphic.element;
if (el.tagName === 'circle') {
el.setAttribute('r', 20);
el.setAttribute('stroke', '#ffffff');
el.setAttribute('stroke-width', '4');
}
}
});
}
}, 1500);
"""
plots/arc-basic/implementations/highcharts.py:167
- The code downloads Highcharts JavaScript files with retry logic (5 attempts with exponential backoff), but this only catches HTTPError exceptions. Network errors (URLError, timeout) and other exceptions will still cause the script to fail without retries. Consider catching broader exceptions or at least urllib.error.URLError to make the retry logic more robust.
for attempt in range(5):
try:
with urllib.request.urlopen(url, timeout=30) as resp:
content = resp.read().decode("utf-8")
cache_path.write_text(content, encoding="utf-8")
js_scripts[name] = content
break
except urllib.error.HTTPError:
time.sleep(3 * (attempt + 1))
plots/arc-basic/metadata/highcharts.yaml:9
- The library version in metadata (1.10.3) refers to the Python highcharts_core package version, but this implementation doesn't use highcharts_core. Instead, it downloads JavaScript from CDN at version 11.4.8. This creates a mismatch between the metadata and the actual implementation. The library_version field should reflect the actual Highcharts JavaScript library version being used (11.4.8), not a hypothetical Python package version.
library_version: 1.10.3
Comment on lines
+99
to
+100
| "nodeFormat": "{point.name}: {point.sum} exchanges", | ||
| "pointFormat": "{point.fromNode.name} \u2192 {point.toNode.name}: {point.weight} exchanges", |
There was a problem hiding this comment.
The tooltip displays scaled weight values (line 100), not the original edge weights. Since weights are multiplied by 8 on line 72, a tooltip showing an edge with original weight 5 will display "40 exchanges" instead of "5 exchanges", which misrepresents the actual data. Either avoid scaling the data or use a tooltip formatter that divides by the scale factor to show true values.
| js_scripts[name] = content | ||
| break | ||
| except urllib.error.HTTPError: | ||
| time.sleep(3 * (attempt + 1)) |
There was a problem hiding this comment.
If all 5 download retry attempts fail (line 159-167), the code continues execution and will crash at lines 168-170 when accessing dictionary keys that don't exist in js_scripts. The retry loop should either raise an exception after exhausting retries or check if the download succeeded before proceeding.
Suggested change
| time.sleep(3 * (attempt + 1)) | |
| time.sleep(3 * (attempt + 1)) | |
| else: | |
| raise RuntimeError(f"Failed to download '{name}' from {url} after 5 attempts") |
Contributor
AI Review - Attempt 1/3Image Description
Score: 82/100
Visual Quality (25/30)
Design Excellence (12/20)
Spec Compliance (15/15)
Data Quality (14/15)
Code Quality (9/10)
Library Mastery (7/10)
Score Caps Applied
Strengths
Weaknesses
Issues Found
AI Feedback for Next Attempt
Verdict: REJECTED |
github-actions
Bot
added
quality:82
github-actions
Bot
added
ai-attempt-1
Attempt 1/3 - fixes based on AI review
Contributor
🔧 Repair Attempt 1/3Applied fixes based on AI review feedback. Status: Repair completed, re-triggering review... |
Contributor
AI Review - Attempt 2/3Image Description
Score: 87/100
Visual Quality (27/30)
Design Excellence (15/20)
Spec Compliance (14/15)
Data Quality (14/15)
Code Quality (10/10)
Library Mastery (7/10)
Score Caps Applied
Strengths
Weaknesses
Issues Found
AI Feedback for Next Attempt
Verdict: REJECTED |
github-actions
Bot
added
quality:87
Attempt 2/3 - fixes based on AI review
Contributor
🔧 Repair Attempt 2/3Applied fixes based on AI review feedback. Status: Repair completed, re-triggering review... |
Contributor
AI Review - Attempt 3/3Image Description
Score: 85/100
Visual Quality (26/30)
Design Excellence (14/20)
Spec Compliance (15/15)
Data Quality (14/15)
Code Quality (9/10)
Library Mastery (7/10)
Score Caps Applied
Strengths
Weaknesses
Issues Found
AI Feedback for Next Attempt
Verdict: REJECTED |
github-actions
Bot
added
quality:85
Attempt 3/3 - fixes based on AI review
Contributor
🔧 Repair Attempt 3/3Applied fixes based on AI review feedback. Status: Repair completed, re-triggering review... |
Contributor
AI Review - Attempt 3/3Image Description
Score: 87/100
Visual Quality (26/30)
Design Excellence (14/20)
Spec Compliance (15/15)
Data Quality (14/15)
Code Quality (10/10)
Library Mastery (8/10)
Score Caps Applied
Strengths
Weaknesses
Issues Found
AI Feedback for Next Attempt
Verdict: REJECTED |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
New highcharts implementation for arc-basic.
Changes
Test Plan
Generated with Claude Code
/updatecommand